It would be better to rename m_maxFPS to make it more relevant to the rendering FPS.

m_maxRenderFPS for example.

For the logic one, it would be better to have the naming match the rendering.

m_maxLogicFPS etc.

I did not intent to rename m_maxFPS for this change to have a bit less diff.

m_maxLogicFPS is not the correct terminology for this. Logic FPS is what we currently refer to as enum LOGICFRAMES_PER_SECOND=30.

m_maxLogicFPS is not the correct terminology for this. Logic FPS is what we currently refer to as enum LOGICFRAMES_PER_SECOND=30.

It's the currently set max logic frame rate, LOGICFRAMES_PER_SECOND is the default maximum value. But since we can / need to be able to vary the current max logic frame rate, it makes sense to call it that for the variable.

m_logicTimeScaleFPS is conceptually not equivalent to LOGICFRAMES_PER_SECOND or m_maxLogicFPS. It effectively is a ratio that scales the logic fps.

You ask me to rename it to m_maxLogicFPS but I am telling you this is not the right name for it. Originally I used this exact name for it, until I realized it is misleading because it would be pretty much identical to LOGICFRAMES_PER_SECOND but are not the same thing. This is why I called it Logic Time Scale.

Currently Logic Time Scale is capped by the Render Update. We could perhaps also uncap it and make it a substitute for fast forwarding globally.

It's not misleading though, i think you are putting too much emphasis on what LOGICFRAMES_PER_SECOND means compared to what it actually is meant to be.

LOGICFRAMES_PER_SECOND is the default maximum logic frame rate during normal gameplay etc. It's basically a value used for default configuration and normal configuration.
While your m_logicTimeScaleFps is the max logic FPS that is being used within the game at runtime. Which can vary to allow fast gameplay mode in skirmish or within mod maps etc.

Both values are related but mean different things.

Logic tick rate being capped by the rendering is fine, but that's a different problem overall. The logic tick rate does not need to exceed the rendering rate and probably never should. But varying the max logic rate implements the fast forward functionality, so of course the render rate has to increase if the logic rate was to exceed it.

Thing is, when we make LOGICFRAMES_PER_SECOND configurable, then what will be its name? Given your name proposal, it would end up being something like:

Int m_logicFPS;
Int m_maxLogicFPS;

I disagree with giving these 2 things the same name.

This will be better:

Int m_logicFPS;
Int m_logicTimeScaleFPS;

The logic tick rate does not need to exceed the rendering rate and probably never should.

It does so in the original game, during fast forwarding. I think it is fine to do that. I can explore this in a follow up change.

Thing is, when we make LOGICFRAMES_PER_SECOND configurable, then what will be its name?

At that point it would be like with any other configurable variable. we have the config handling set Int m_maxLogicFPS; at startup etc. Or it uses the constant LOGICFRAMES_PER_SECOND as the default.

I expect it will be possible to change LOGICFRAMES_PER_SECOND at runtime to any value above 0.

Does fast forwarding actually work in replays?

The logic here looks like it does not affect the logic rate so would actually break replay fast forwarding.

Last time I checked it worked, but I can retest.

This is fine in this update then if you have the logic frame rate limiting disabled by default.

But once it does get enabled it will break fast forwarding at this point.

I tested with capped and uncapped logic time scale and fast forward worked.

I made a pass on the fast forward implementation and made sure that the new code does not break it. I tested and it worked. The plan forward is to replace the fast forward code with the new logic time scale function when the logic update can exceed the render update. I hope it won't be that hard to do.

I think as long as the render logic is not being locked then the fast forward here should still function okay.

it's just when people decide to lock the logic but unlock the rendering, the tivo_mode variable might need checking as well in the logic rate limiting to disable / ignore the limiting when it is active it.

I don't quite follow. Do I need to do anything?

I don't quite follow. Do I need to do anything?

Ah i see you catored for it in the update for the logic

Bool fastForward = !TheGameLogic->isGamePaused() && TheGlobalData->m_TiVOFastMode && TheGameLogic->isInReplayGame();
#endif
if (fastForward)
{
	TheGameClient->step();
	TheGameLogic->UPDATE();
}

I didn't notice that yesterday so might have been what you just added.

But this could be made simpler by appending the fastForward check to the if statement above it were you check to see if the locking is enabled and the timestep has been reached.
so we either go if we are fastforwarding or are enabled and the timestep has been met.

Yes. I added it today.

But this could be made simpler

Fixed

This is not correct, the value being divided by the FPS needs to be whatever the current logic rate is otherwise the scrolling will be inconsistent during fast forward and fast game speeds.

I tested it and it works correctly.

Using TheGlobalData->m_framesPerSecondLimit is not correct because the fps setting in GameData.ini or -fps command line argument should not affect the scroll speed.